Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adjust wide_counter_csr_t::written_value() to only increment if counting is enabled, Bug Fix for PR 1381 #1581

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rbuchner-aril
Copy link
Contributor

There looks to be a bug in the wide_counter_csr_t::written_value() when is_counting_enabled() == false. This bug was introduced in PR 1381, specifically in c927773.

The control flow for this is:

void csr_t::write(const reg_t val) noexcept {
  const bool success = unlogged_write(val);
  if (success) {
    log_write();
  }
}

First calls unlogged_write()

bool wide_counter_csr_t::unlogged_write(const reg_t val) noexcept {
  this->val = val;
  // The ISA mandates that if an instruction writes instret, the write
  // takes precedence over the increment to instret.  However, Spike
  // unconditionally increments instret after executing an instruction.
  // Correct for this artifact by decrementing instret here.
  // Ensure that Smctrpmf hasn't disabled counting.
  if (is_counting_enabled()) {
    this->val--;
  }
  return true;
}

If !is_counting_enabled(), then this->val is not decremented.

However, log_write() calls written_value()

void csr_t::log_write() const noexcept {
  log_special_write(address, written_value());
}
reg_t wide_counter_csr_t::written_value() const noexcept {
  // Re-adjust for upcoming bump()
  return val + 1;
}

Which reports back an adjust version of val regardless.

@atulkharerivos could you please review this change

…ing is enabled

The stored value is only decremented in that case so only then does it need re-adjusted.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant